Skip to content

Optimize out metadata for typeof(X).IsValueType and .IsEnum #118528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MichalStrehovsky
Copy link
Member

We don't need reflection metadata to answer this.

Change salvaged out of #118479, rt-sz will decide if we want it.

We don't need reflection metadata to answer this.

Change salvaged out of dotnet#118479, rt-sz will decide if we want it.
@Copilot Copilot AI review requested due to automatic review settings August 8, 2025 12:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR optimizes the AOT compilation process by avoiding the generation of unnecessary reflection metadata for simple type queries. The optimization specifically targets patterns like typeof(X).IsValueType and typeof(X).IsEnum where the answer can be determined at compile time without requiring full reflection metadata.

Key changes:

  • Adds pattern detection for typeof(X).IsValueType and typeof(X).IsEnum call sequences
  • Modifies helper ID assignment to use NecessaryTypeHandle instead of full metadata when these patterns are detected

Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky
Copy link
Member Author

The savings are borderline, but maybe worth it given the low complexity:

Size statistics

Pull request #118528

Project Size before Size after Difference
TodosApi-linux 24216856 24183768 -33088
TodosApi-windows 25530368 25500672 -29696
avalonia.app-linux 18799408 18799408 0
avalonia.app-windows 19328512 19328512 0
hello-linux 1364944 1364944 0
hello-minimal-linux 1094456 1094456 0
hello-minimal-windows 840192 840192 0
hello-windows 1138176 1138176 0
kestrel-minimal-linux 5201560 5180808 -20752
kestrel-minimal-windows 4746240 4727296 -18944
reflection-linux 1964096 1964096 0
reflection-windows 1823744 1823744 0
webapiaot-linux 9682544 9653568 -28976
webapiaot-windows 10245632 10218496 -27136
winrt-component-full-windows 6678528 6678528 0
winrt-component-minimal-windows 1708544 1708032 -512

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

{
if (reader.HasNext
&& reader.ReadILOpcode() is ILOpcode.callvirt or ILOpcode.call
&& _methodIL.GetObject(reader.ReadILToken()) is MethodDesc { Name: "get_IsValueType" or "get_IsEnum"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add IsPrimitive too? It is used in some places in BCL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see what rt-sz says. We can do this for several other properties but I wasn't sure how much of a coupling between the compiler and the BCL we are okay with in this area.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to avoid coupling between the compiler and the BCL, we would make this pattern must-expand in the JIT. It would reduce the coupling to JIT/scanner that exists in many places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test coverage for the (rare) situation when the JIT does not expand this pattern as an intrinsic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to avoid coupling between the compiler and the BCL, we would make this pattern must-expand in the JIT. It would reduce the coupling to JIT/scanner that exists in many places.

Hmm, I haven't even though about it in the context of the JIT optimization.

My original reason for why we don't need the metadata is because the implementation in RuntimeType will just go straight to the MethodTable if there is one and ask there. But yeah, RyuJIT will expand it before that, so we won't even get there.

I guess the only way to ensure this codepath really works would be to run unoptimized mode with the scanner enabled. I don't really know if I want to add a test pass with that. Or make it MustExpand in RyuJIT but not sure if that would be okay (cc @EgorBo).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand how e.g. IsPrimitive can be a must-expand intrinsic -- ho do we expand it for bool Foo(Type t) => t.IsPrimitive; ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have to be a special "must expand if this comes from a typeof" rule (this is the logic that scanner is doing.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that will work, but it kind of relies that JIT doesn't spill typeof to a locally anyhow (e.g. under some stress)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the patch to do that:

From b9ee418054231419e5cbd0e7a8f8da1a329758d5 Mon Sep 17 00:00:00 2001
From: EgorBo <[email protected]>
Date: Tue, 12 Aug 2025 16:55:12 +0200
Subject: [PATCH] make typeof().IsX must expand

---
 src/coreclr/jit/importercalls.cpp | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp
index a4e2e454b96..431668fc403 100644
--- a/src/coreclr/jit/importercalls.cpp
+++ b/src/coreclr/jit/importercalls.cpp
@@ -3500,6 +3500,19 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE    clsHnd,
                 mustExpand = true;
                 break;
 
+            case NI_System_Type_get_IsEnum:
+            case NI_System_Type_get_IsValueType:
+            case NI_System_Type_get_IsByRefLike:
+            case NI_System_Type_get_IsPrimitive:
+                {
+                    CORINFO_CLASS_HANDLE hClass = NO_CLASS_HANDLE;
+                    if (gtIsTypeof(impStackTop().val, &hClass))
+                    {
+                        mustExpand = true;
+                    }
+                }
+                break;
+
             case NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference:
                 mustExpand |= sig->sigInst.methInstCount == 1;
                 break;
-- 
2.45.2.windows.1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if this gets sketchy, we can just come back to #118528 (comment): "but maybe worth it given the low complexity" - if the complexity is in ensuring test coverage in corner cases, this makes it no longer "low complexity" and maybe not worth it.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants